Skip to content

Migrate tests in google/cloud/sensors from unittest to pytest - #29642

Merged
dimberman merged 1 commit into
apache:mainfrom
vedantlodha:google-sensor-ut
Feb 27, 2023
Merged

Migrate tests in google/cloud/sensors from unittest to pytest#29642
dimberman merged 1 commit into
apache:mainfrom
vedantlodha:google-sensor-ut

Conversation

@vedantlodha

@vedantlodha vedantlodha commented Feb 20, 2023

Copy link
Copy Markdown
Contributor

This change removes dependencies on unittest.TestCase from google/cloud/sensors package. The use of unittest in this package is now limited to only unittest.mock which is fine acc to the original issue.

Tested using breeze testing tests --test-type "Providers[google]"

related #29305


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg Bot added area:providers provider:google Google (including GCP) related issues labels Feb 20, 2023
@vedantlodha

Copy link
Copy Markdown
Contributor Author

@Taragolis @potiuk , another PR for a subtask of issue #29305. This doesnt entirely close the issue, some packages still remain to migrate. TIA

@potiuk

potiuk commented Feb 20, 2023

Copy link
Copy Markdown
Member

@Taragolis @potiuk , another PR for a subtask of issue #29305. This doesnt entirely close the issue, some packages still remain to migrate. TIA

Change the Fixes: #PR into Related: #PR - otherwise it will close the issue again automatically @vedantlodha 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're switching from unittest to pytest then why are we even using test classes at all? We might as well go all the way and make the tests into functions. I'd rather make this change now while we're already modifying the code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that classes vs functions it is not new tabs vs spaces 🤣
Personally I do not have any preferences, outside of Airflow I use both of them depends on situation.

If module contain a lot of different functions / classes which need to be tested then classes very nice to use as namespace so developers could easily found tests for specific part, like:

class TestAwesomeHook:

    def test_init(self, ...):
        ...

    def test_init_failure_reason_1(self, ...):
        ...

    def test_get_conn(self, ...):
        ...

class TestAnotherAwesomeHook:

    def test_init(self, ...):
        ...

    def test_init_failure_reason_1(self, ...):
        ...

    def test_get_conn(self, ...):
        ...

vs

def test_awesome_hook_init(...):
    ...

def test_awesome_hook_init_failure_reason_1(...):
    ...

def test_another_awesome_hook_init(...):
    ...

def test_another_awesome_hook_get_conn(...):
   ...

def test_awesome_hook_init_failure_reason_1(...):
    ...

def test_awesome_hook_get_conn(...):
   ...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

especially in the case of providers, I feel that class-based tests help because we have many operators/sensors in a single file so if tests are class-based it improves the readability of tests and adding/removing/updating/searching tests became a bit easy. otherwise, If I'll make small changes in the existing operator most of the time goes in figuring out do I need to add new tests or can update existing and where shall I add the new tests in a test file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! Tried retaking a look, and in my opinion as well, having class names for sensors makes the tests lot more readable. @dimberman I would request you to re-evaluate your request to remove classes. Thanks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh ok that makes sense. I'm always a fan of minimizing nesting as much as possible but I think this is reasonable considering there are multiple classes in the file

This change removes dependencies on unittest.TestCase from google/cloud/sensors package. The use of unittest in this package is now limited to only unittest.mock .

Related: apache#29305
@dimberman
dimberman merged commit 96dd371 into apache:main Feb 27, 2023
@vedantlodha
vedantlodha deleted the google-sensor-ut branch February 28, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants